-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: UI improvements #318
base: master
Are you sure you want to change the base?
feat: UI improvements #318
Conversation
Thanks for the pull request, @Lunyachek! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
1802de1
to
75ed23b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #318 +/- ##
==========================================
+ Coverage 70.73% 70.87% +0.14%
==========================================
Files 28 28
Lines 410 412 +2
Branches 87 89 +2
==========================================
+ Hits 290 292 +2
Misses 119 119
Partials 1 1 ☔ View full report in Codecov by Sentry. |
- Minor fix for Back to My Records button - Decrease font size for the Questions about Learner Records title - Send Program Record modal window - checkboxes alignment
75ed23b
to
e1accfe
Compare
Hi @openedx/2u-aperture! This PR and it's backport are ready for review. Thanks! |
Is this the kind of thing that requires product review (because it's a design change)? And if so, did it get one? (forgive me if I don't understand what needs to go through product review; I'm trying to get a handle on this.) |
@deborahgu I'll check with Product to see! |
@@ -6,13 +6,13 @@ import { Hyperlink } from '@openedx/paragon'; | |||
function RecordsHelp({ helpUrl }) { | |||
return ( | |||
<section className="help"> | |||
<h2> | |||
<h3 className="h5"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you switching to a skipped heading level here? The previous heading is h1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/components/ProgramRecordSendModal/SendLearnerRecordModal.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the fixes. Once this is approved by @deborahgu, I'll take a look at the backport PR too.
@deborahgu Any additional feedback or suggestions here? Are there any other changes you'd like to see? |
|
Hi @deborahgu and @justinhynes! I checked in again with @jmakowski1123 and she said this should go through Product review. I've added the label and we'll hold for her team to take a look. |
Dismissing review until this has gone through Product review.
I removed the |
Hi @mphilbrick211 & @jmakowski1123! Just following up on this since it's been awhile. Did this ever go through Product Review? |
Hello everyone, |
This is still awaiting product review (cc @mphilbrick211). And once it's had product review, we're going to require the skipped heading level be addressed for accessibility purposes. Copying broken prior art is still broken. |
Minor fix for Back to My Records button
First proposal: make the button look the same as the button on the records page
Second proposal: remove the
<a>
tag from the<button>
tag. The current approach is not recommended due to potential accessibility and semantic issuesDecrease font size for the
Questions about Learner Records
title - to match with the same title on Records pageSend Program Record modal window - checkboxes alignment
Before
After